Change ASTUnit::getASTContext() const to return a non-const ASTContext
#130096
Conversation
…text` Also, remove the non-const `ASTUnit::getASTContext()` since it's no longer necessary. This would make it similar to `Decl::getAstContext() const` for a more consistent and flexible API.
|
@llvm/pr-subscribers-clang Author: Boaz Brickner (bricknerb) ChangesAlso, remove the non-const Full diff: https://github.com/llvm/llvm-project/pull/130096.diff 1 Files Affected:
diff --git a/clang/include/clang/Frontend/ASTUnit.h b/clang/include/clang/Frontend/ASTUnit.h
index 1f98c6ab328ba..bd55c8b627941 100644
--- a/clang/include/clang/Frontend/ASTUnit.h
+++ b/clang/include/clang/Frontend/ASTUnit.h
@@ -439,8 +439,7 @@ class ASTUnit {
Preprocessor &getPreprocessor() { return *PP; }
std::shared_ptr<Preprocessor> getPreprocessorPtr() const { return PP; }
- const ASTContext &getASTContext() const { return *Ctx; }
- ASTContext &getASTContext() { return *Ctx; }
+ ASTContext &getASTContext() const { return *Ctx; }
void setASTContext(ASTContext *ctx) { Ctx = ctx; }
void setPreprocessor(std::shared_ptr<Preprocessor> pp);
|
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
This is weakening const correctness, which is the opposite direction of where we should be going IMO.
Endilll
left a comment
Endilll
left a comment
There was a problem hiding this comment.
I agree with Aaron that this patch doesn't move us towards the direction we generally want. I'd like to see more justification than 2 lines in the description.
|
There is no const_cast, or casting away const in any form on this patch, so how does it weaken const correctness? |
|
To put it another way, what do we want to prevent the user from accomplishing if he obtains a non-const reference to ASTContext from a const ASTUnit? |
When a function is marked as |
|
Thanks for the feedback! Is there a way to call However, when I have a |
There's two answers. In a world where we have const correctness, you shouldn't be able to (this is a misfeature on |
shafik
left a comment
shafik
left a comment
There was a problem hiding this comment.
Just throwing in my $0.02 and saying I agree w/ Aaron and Vlad here, this is not the right direction. Having a const method return a non-const that we will then mutate is just not clean code.
Having to use const_cast documents we are violating expectations and marks this as something we need to clean up in the future.
|
Thanks everyone! |
This is a followup of [a comment](https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815) in carbon-language#5062. Change `File::cpp_ast()` to be `const` while keeping it returning a non-const pointer to `clang::ASTUnit`. This makes the fact we use [Clang with lack of const correctness](llvm/llvm-project#130096 (comment)) explicit. Alternatives: * Change `ASTUnit::getASTContext() const` to return a non-const `ASTContext`. [Rejected due to weakening const correctness](llvm/llvm-project#130096). * Change `createMangleContext()` to be `const`. Tried that and it seems like it relies heavily on non const API. * Change Clang `MangleContext::mangleName()` to `const`. * Use `const_cast` on `ASTContext` when calling `createMangleContext()`.
This is a followup of [a comment](https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815) in #5062. Add a mutable AST pointer to `FileContext`. This is necessary since we use [Clang with lack of const correctness](llvm/llvm-project#130096 (comment)). Alternatives in Clang: * Change `ASTUnit::getASTContext() const` to return a non-const `ASTContext`. [Tried and was rejected upstream due to weakening const correctness](llvm/llvm-project#130096). * Change `createMangleContext()` to be `const`. Tried that and it seems like it relies heavily on non const API. * Change `MangleContext::mangleName()` to `const`. Tried that but there are several lazy initialization and id creations happening that modify the context. See details in llvm/llvm-project#130613. Alternatives in Carbon: * Use `const_cast` on `ASTContext` when calling `createMangleContext()`. * Make `FileContext::sem_ir_` point to a mutable `SemIR::File`. * Change `File::cpp_ast()` to be const while keeping it return a mutable pointer. Part of #4666.
6 participants
Also, remove the non-const
ASTUnit::getASTContext()since it's no longer necessary.This would make it similar to
Decl::getAstContext() constfor a more consistent and flexible API.